Use Connection() from util package #123

jsafrane · 2019-02-08T09:55:03Z

This has several effects:

--connection-timeout option has no effect now, Connect() waits forever for the driver.
Attacher exits when driver socket is closed (i.e. driver container either dies or restarts). This makes sure that attacher can cache driver capabilities safely, as we don't expect that driver can change its capabilities while it's running.

@pohly, @msau42, PTAL

k8s-ci-robot · 2019-02-08T09:55:11Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jsafrane

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [jsafrane]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

pohly

I've tried this with the example hostpath deployment. It works, but I noticed one drawback: when the hostpath driver gets removed, the csi-attacher container immediately exists and then also immediately gets restarted.

That's all as intended, but it also has the effect that the "Lost connection to CSI driver, exiting" message is lost. kubectl logs only shows the logs of the restarted csi-attacher.

Should we change that by logging the exit reason to /dev/termination-log (https://kubernetes.io/docs/tasks/debug-application-cluster/determine-reason-pod-failure/#writing-and-reading-a-termination-message)?

pohly · 2019-02-08T12:01:17Z

Another solution is to change the deployment:

diff --git a/deploy/hostpath/csi-hostpath-attacher.yaml b/deploy/hostpath/csi-hostpath-attacher.yaml
index df7f0d63..534ff632 100644
--- a/deploy/hostpath/csi-hostpath-attacher.yaml
+++ b/deploy/hostpath/csi-hostpath-attacher.yaml
@@ -32,6 +32,7 @@ spec:
         - name: csi-attacher
           image: quay.io/k8scsi/csi-attacher:v1.0.1
           imagePullPolicy: Always
+          terminationMessagePolicy: FallbackToLogsOnError
           args:
             - --v=5
             - --csi-address=$(ADDRESS)

But the result isn't as nice:

    Last State:  Terminated
      Reason:    Error
      Message:   ion.go:157] GRPC request: {}
I0208 11:59:10.391492       1 connection.go:159] GRPC response: {"capabilities":[{"Type":{"Service":{"type":1}}}]}
I0208 11:59:10.393149       1 connection.go:160] GRPC error: <nil>
I0208 11:59:10.393162       1 connection.go:156] GRPC call: /csi.v1.Controller/ControllerGetCapabilities
I0208 11:59:10.393169       1 connection.go:157] GRPC request: {}
I0208 11:59:10.395096       1 connection.go:159] GRPC response: {"capabilities":[{"Type":{"Rpc":{"type":1}}},{"Type":{"Rpc":{"type":5}}},{"Type":{"Rpc":{"type":6}}}]}
I0208 11:59:10.397978       1 connection.go:160] GRPC error: <nil>
I0208 11:59:10.397993       1 main.go:154] CSI driver does not support ControllerPublishUnpublish, using trivial handler
I0208 11:59:10.398189       1 controller.go:113] Starting CSI attacher
I0208 11:59:10.398580       1 reflector.go:131] Starting reflector *v1.PersistentVolume (10m0s) from k8s.io/client-go/informers/factory.go:132
I0208 11:59:10.398634       1 reflector.go:169] Listing and watching *v1.PersistentVolume from k8s.io/client-go/informers/factory.go:132
I0208 11:59:10.398588       1 reflector.go:131] Starting reflector *v1beta1.VolumeAttachment (10m0s) from k8s.io/client-go/informers/factory.go:132
I0208 11:59:10.398652       1 reflector.go:169] Listing and watching *v1beta1.VolumeAttachment from k8s.io/client-go/informers/factory.go:132
I0208 11:59:10.498517       1 shared_informer.go:123] caches populated
I0208 11:59:10.498679       1 controller.go:175] Started VA processing "csi-86113377f1338d05e24b17447d9a07fdcb35f8cf23bef352d63c43e00793fef5"
I0208 11:59:10.498713       1 controller.go:190] Skipping VolumeAttachment csi-86113377f1338d05e24b17447d9a07fdcb35f8cf23bef352d63c43e00793fef5 for attacher pmem-csi
I0208 11:59:10.498746       1 controller.go:205] Started PV processing "pvc-cde28d4c-2b8e-11e9-82fb-deadbeef0100"
E0208 11:59:24.983978       1 connection.go:105] Lost connection to unix:///csi/csi.sock.
F0208 11:59:24.983998       1 connection.go:87] Lost connection to CSI driver, exiting

      Exit Code:    255

pohly · 2019-02-08T12:02:38Z

Here's a patch for the code:

diff --git a/pkg/connection/connection.go b/pkg/connection/connection.go
index a2abf84..1d1d86d 100644
--- a/pkg/connection/connection.go
+++ b/pkg/connection/connection.go
@@ -19,6 +19,7 @@ package connection
 import (
        "context"
        "fmt"
+       "io/ioutil"
 
        "github.com/container-storage-interface/spec/lib/go/csi"
        "github.com/kubernetes-csi/csi-lib-utils/connection"
@@ -84,7 +85,12 @@ func New(address string) (CSIConnection, error) {
 }
 
 func onReconnect() bool {
-       klog.Fatalf("Lost connection to CSI driver, exiting")
+       terminationMsg := "Lost connection to CSI driver, exiting"
+       terminationLog := "/dev/termination-log"
+       if err := ioutil.WriteFile(terminationLog, []byte(terminationMsg), 0644); err == nil {
+               klog.Errorf("%s: %s", terminationLog, err)
+       }
+       klog.Fatalf(terminationMsg)
        return false
 }

This leads to this exit status:

    Last State:     Terminated
      Reason:       Error
      Message:      Lost connection to CSI driver, exiting
      Exit Code:    255
      Started:      Fri, 08 Feb 2019 12:56:19 +0100
      Finished:     Fri, 08 Feb 2019 12:56:30 +0100

msau42

lgtm just a nit. And I like @pohly 's idea of for the termination msg

msau42 · 2019-02-08T21:01:06Z

Gopkg.toml

@@ -44,7 +44,7 @@

 [[constraint]]
  name = "github.com/kubernetes-csi/csi-lib-utils"
-  version = "0.1.0"
+  version = "0.3.0"


do we want to constrain the version here or just use the latest?

I'm for "use the latest" and the constraint does allow that because it means ">= 0.3.0". So this looks right to me.

jsafrane · 2019-02-11T11:30:27Z

I like the terminationLog usage, however, it belongs to csi-lib-utils, because it's going to be used in provisioner too.

jsafrane · 2019-02-11T12:42:37Z

Created kubernetes-csi/csi-lib-utils#16 to move GetDriverName to -util.

jsafrane · 2019-02-11T13:27:30Z

/hold
Until csi-test is released with kubernetes-csi/csi-test#165. Right now, Connect() does not support tcp addresses that csi-test uses.

jsafrane · 2019-02-14T14:39:31Z

Filled kubernetes-csi/csi-lib-utils#18 for writing termination log.

msau42 · 2019-02-14T22:59:51Z

https://github.com/kubernetes-csi/csi-test/releases/tag/v1.1.1

jsafrane · 2019-02-15T13:44:43Z

I rewrote the PR to newest csi-test (that pulls container-storage-interface/spec@master; the first commit) + my current PRs in csi-lib-utils (the second commit) to show how the code is going to look when everything gets merged.

And I tested everything with a mock driver.

msau42 · 2019-02-16T02:47:25Z

Try this: https://github.com/kubernetes-csi/csi-test/releases/tag/v1.0.3-rc1

msau42 · 2019-02-20T19:50:37Z

https://github.com/kubernetes-csi/csi-lib-utils/releases/tag/v0.3.1-rc1

jsafrane · 2019-02-21T13:46:00Z

Rebased to csi-lib-utils=0.3.1-rc1 for preview. Waiting for official release.

msau42 · 2019-02-21T20:13:37Z

https://github.com/kubernetes-csi/csi-lib-utils/releases/tag/v0.3.1
https://github.com/kubernetes-csi/csi-test/releases/tag/v1.0.3

jsafrane · 2019-02-22T15:14:06Z

Rebased to csi-test 0.3.1 and csi-lib-util 1.0.3

jsafrane · 2019-02-22T17:39:27Z

/hold cancel

msau42 · 2019-02-22T22:03:07Z

/lgtm

k8s-ci-robot requested review from davidz627 and msau42 February 8, 2019 09:55

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 8, 2019

pohly reviewed Feb 8, 2019

View reviewed changes

This was referenced Feb 8, 2019

node driver registrar should use CSI connection lib kubernetes-csi/node-driver-registrar#29

Merged

use CSI connection lib kubernetes-csi/cluster-driver-registrar#32

Merged

msau42 reviewed Feb 8, 2019

View reviewed changes

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 11, 2019

jsafrane mentioned this pull request Feb 14, 2019

Add StartOnAddress function to listen on unix:// paths kubernetes-csi/csi-test#165

Merged

Bump csi-test and csi-lib-util

20c91d6

jsafrane force-pushed the use-lib-connection branch from e2cd472 to 9c8de49 Compare February 15, 2019 13:35

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 15, 2019

jsafrane force-pushed the use-lib-connection branch from 9c8de49 to 2528d33 Compare February 15, 2019 13:44

jsafrane force-pushed the use-lib-connection branch from 2528d33 to 3c3bdec Compare February 21, 2019 13:44

k8s-ci-robot removed the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Feb 21, 2019

k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Feb 21, 2019

jsafrane added 2 commits February 22, 2019 16:11

Use common functions from util package

795b662

Fix unit tests

ff48934

jsafrane force-pushed the use-lib-connection branch from 3c3bdec to ff48934 Compare February 22, 2019 15:13

jsafrane mentioned this pull request Feb 22, 2019

Use Connection() from util package kubernetes-csi/external-provisioner#234

Merged

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 22, 2019

k8s-ci-robot assigned msau42 Feb 22, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 22, 2019

k8s-ci-robot merged commit 0e6f5f2 into kubernetes-csi:master Feb 22, 2019

dmitrii-didenko mentioned this pull request Dec 15, 2023

Csi-attacher Looses Connection to Driver Unix Socket kubernetes-sigs/aws-ebs-csi-driver#1875

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use Connection() from util package #123

Use Connection() from util package #123

jsafrane commented Feb 8, 2019

k8s-ci-robot commented Feb 8, 2019

pohly left a comment •

edited

Loading

pohly commented Feb 8, 2019

pohly commented Feb 8, 2019

msau42 left a comment

msau42 Feb 8, 2019

pohly Feb 11, 2019

jsafrane commented Feb 11, 2019

jsafrane commented Feb 11, 2019

jsafrane commented Feb 11, 2019

jsafrane commented Feb 14, 2019

msau42 commented Feb 14, 2019

jsafrane commented Feb 15, 2019

msau42 commented Feb 16, 2019

msau42 commented Feb 20, 2019

jsafrane commented Feb 21, 2019

msau42 commented Feb 21, 2019

jsafrane commented Feb 22, 2019

jsafrane commented Feb 22, 2019

msau42 commented Feb 22, 2019

Use Connection() from util package #123

Use Connection() from util package #123

Conversation

jsafrane commented Feb 8, 2019

k8s-ci-robot commented Feb 8, 2019

pohly left a comment • edited Loading

Choose a reason for hiding this comment

pohly commented Feb 8, 2019

pohly commented Feb 8, 2019

msau42 left a comment

Choose a reason for hiding this comment

msau42 Feb 8, 2019

Choose a reason for hiding this comment

pohly Feb 11, 2019

Choose a reason for hiding this comment

jsafrane commented Feb 11, 2019

jsafrane commented Feb 11, 2019

jsafrane commented Feb 11, 2019

jsafrane commented Feb 14, 2019

msau42 commented Feb 14, 2019

jsafrane commented Feb 15, 2019

msau42 commented Feb 16, 2019

msau42 commented Feb 20, 2019

jsafrane commented Feb 21, 2019

msau42 commented Feb 21, 2019

jsafrane commented Feb 22, 2019

jsafrane commented Feb 22, 2019

msau42 commented Feb 22, 2019

pohly left a comment •

edited

Loading